fix(tile_layer): dispose the image handed to a tile pruned during dispatch - #2239
Open
dinin92-del wants to merge 4 commits into
Open
fix(tile_layer): dispose the image handed to a tile pruned during dispatch#2239dinin92-del wants to merge 4 commits into
dinin92-del wants to merge 4 commits into
Conversation
added 4 commits
August 5, 2026 12:02
…patch
`TileImage._onImageLoadSuccess` stored the `ImageInfo` even when the tile was
already disposed. `ImageStreamCompleter.setImage` gives every listener its own
handle and the listener owns it: a displayed tile passes ownership to
`RenderImage`, which disposes it, but a disposed tile never builds one — so the
decoded image stays alive for the lifetime of the process.
`dispose()` does remove the listener, which is why this is not reachable by
simply disposing a tile and then completing its image. It is reachable because
`setImage` dispatches over a copy of the listener list ("Make a copy to allow
for concurrent modification"): a listener removed from inside that loop is
still called. Two tiles resolving equal keys share one completer, and
`onLoadComplete` is where tiles get pruned — as already noted in
`TileImageManager.reloadImages` — so a tile can be disposed mid-dispatch and
handed an image regardless.
Unnoticeable with 256x256 tiles (256 KB). With 768x768 RGBA tiles (2.25 MB
each) it killed an app on iOS while browsing the map.
Adds a regression test that reproduces the dispatch race; it fails on the
current code with `Expected: null / Actual: ImageInfo:<[8x8] @ 1.0x>`.
Ownership of a tile's decoded image transfers at BUILD time: `RawImage` passes the raw `ui.Image` to `RenderImage`, which disposes it when it is replaced or unmounted. That is why `TileImage.dispose()` never freed `imageInfo` — by then the render object owns it. The model assumes ONE frame per tile. That holds for static tiles, but a completer emitting a second frame (progressive tiles: a base frame, then a composed one) overwrites `imageInfo` before any build has to happen. When both frames land within the same frame budget — the common case on a fast device — the first handle never reaches a `RenderImage` and nothing frees it. Measured on an iPhone with 768x768 composed tiles: ~0.9 leaked handles per tile, growing linearly with tiles browsed, and invisible to `ImageCache`, which reported ~25 live images while 3336 `ui.Image` objects were alive. Two app kills within four minutes. The tile now tracks whether its handle was passed on (`Tile` marks it while building `RawImage`) and frees it only while it is still its own — on frame replacement and on dispose. Freeing a handle the render object owns would be a double free, so the flag is what makes this safe rather than lucky. Two tests, both counting real open handles rather than asserting on code: one for the leaking path, one for the mirror case where the widget already took the handle and the tile must keep its hands off.
…ever takes over The flag-based fix (823e8a8) rested on a false premise. It assumed ownership of the decoded image transfers to `RenderImage` at build time, so it freed the handle only when NO build had taken it. Verified against Flutter sources: `RawImage` CLONES the image for its render object — both `createRenderObject` and `updateRenderObject` pass `image?.clone()`. The render object frees its own clone; the tile's handle never stops being the tile's. Consequence: the flag exempted exactly the PAINTED frames, so every painted frame still leaked one handle. Measured on device after 823e8a8: ~0.3 leaked handles per tile — matching the fraction of tiles that emit two frames — linear, no plateau, invisible to ImageCache. GC finalizers reclaim such handles eventually, which is why 256 KB default tiles get away with the upstream model and 2.25 MB tiles do not. The correct model is simpler than the flag: the tile frees its own handle, always — on frame replacement and on dispose. Dispose also nulls the field, because dispose can race the layer rebuild and a straggler build must see null (paint nothing) rather than clone a disposed image. Caught by a new full-cycle test (TileImage → Tile → RawImage → RenderImage) counting open handles after complete teardown — the pure-TileImage tests could not see it, because the bug lived in what the widget integration does NOT do with the handle. Sensitivity: reverting both files to 823e8a8 fails all three cases of that test.⚠️ Test hygiene that mattered: `createTestImage` caches by size and returns clones of ONE shared image, making two "independent" handle counters move in lockstep. `cache: false` + distinct sizes are load-bearing in these tests.
…ed once
Simplify pass over the leak fix, four angles (reuse / simplification /
efficiency / altitude), behavior untouched — the sensitivity check (both lib
files reverted to the flag model) still fails all three full-cycle cases.
- test_utils/test_frame_driver.dart: ONE DrivenCompleter/DrivenProvider pair
and one testTileImage factory replace three byte-identical class pairs and
two TileImage factories spread over two files. A constructor change now
touches one place.
- _SharedManualImageProvider deleted: OneFrame-wrapping a Completer was a
strict subset of the driven provider.
- The ownership doctrine lives ONCE, on the imageInfo field doc; the
replacement and dispose sites state the rule in one line and point there.
The story already changed once (the flag model) and needed every copy
edited — that class of drift is what this fork patch exists to kill.
- The _disposed branch comment no longer speaks the dead model's language
("never hands its image to a RenderImage") — it argues from ownership:
after dispose() the owner that would free the handle is gone.
- Whitespace orphans of the removed flag mechanism dropped (tile.dart is now
untouched by this branch, as it should be — the fix lives entirely in the
owner).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TileImage._onImageLoadSuccessstores theImageInfoeven when the tile is already disposed.ImageStreamCompleter.setImagegives every listener its own handle and the listener owns it: a displayed tile passes ownership toRenderImage, which disposes it — but a disposed tile never builds one, so the decoded image stays alive for the lifetime of the process.Why
dispose()removing the listener isn't enoughThis is not reachable by simply disposing a tile and then completing its image —
dispose()removes the listener andsetImageearly-returns on an empty listener list.It is reachable because
setImagedispatches over a copy of the listener list:A listener removed from inside that loop is still called. Two tiles resolving equal keys share one completer, and
onLoadCompleteis where tiles get pruned — asTileImageManager.reloadImagesalready notes:So the first tile's completion can dispose the second tile mid-dispatch, and the second tile is handed an image anyway.
Impact
Unnoticeable with 256×256 tiles (256 KB). With 768×768 RGBA tiles (2.25 MB each) it killed an app on iOS while browsing the map — the process was terminated at its memory limit after a few minutes of panning.
Test
Adds
test/layer/tile_layer/tile_image_test.dart, which reproduces the dispatch race with two tiles sharing one completer and asserts both that the disposed tile keeps noImageInfoand that the handle it was given is released (debugGetOpenHandleStackTraces).On the current code it fails with:
Full suite passes with the change (118/118).
AI usage disclosure (per CONTRIBUTING): this patch and its test were written with AI assistance (Claude). I reviewed the change and the reasoning, ran the test suite, and verified that the added test fails on unpatched
masterand passes with the fix. I take responsibility for the code.